-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Events expansion for the progress tracker to provide more detailed information about pin status #38
base: master
Are you sure you want to change the base?
Conversation
@reinerRubin Hey, that was fast! Thanks! 🎉 Also, thanks for putting up a draft version! It helps a lot. Others can weigh in, but I think your intuition about being "complicated" for a first version is on the right track. I can imagine that we will need a robust progress tracker in time, but for now I'd try and figure out a simpler change. What if we start by just reporting the CIDs that we start pinning? I think, in that case, we need to add just a little bit of information to the |
But if I get it right, progress is the consequence of asking the tracker how it is going by ticker. So the tracker has to have some state. As I see tracker can store a slice of planned to pin CIDs and return this slice on demand with flushing his current state. So the pin command can iterate through and emit (AddPinOutput). It was my first idea but I was not sure it's ok enough (plus I am not fun of polling by timer). If it's ok, then I can do this version (it would be like channel but much simpler). |
@reinerRubin Try it out! My only suggestion is to push often so you can get more feedback. Maybe figure out how you will surface these events to the user (like you said originally)? |
3787392
to
8dcc6d9
Compare
@michaelavila, I have tried the approach above and written a test to simulate things that are expected to be in ipfs command. |
@reinerRubin want to take a crack at surfacing these events to the user? I know that one goal is to surface these events during the |
@michaelavila, yeah. I am going to prepare the second PR to the ipfs (pin add command) itself. How must it be organized? Should I make PR with "hacked" vendor and then set actual version after merging this one? // About your comment. I am not sure about "PlanToPin" maybe "Pinning" would fit better. |
You can use the replace directive in the That will work for the time being. Eventually, when you get the Let me know if I should clarify anything or if you run into trouble. |
Ok, got it. |
8dcc6d9
to
3ff70c5
Compare
@michaelavila
I have tried to implement a small events system for the progress tracker. I wanted to expose a channel as a more natural interface for events. But we can not take the risk of blocking a "pin" goroutine. So there is some kind of "infinity" channel inside.
idk, it seems overcomplicated, but exposing a slice of events seemed also terrible.
It's a draft version just to show my idea. I am going to add more tests (and fix race), make it more clear and rename some things. Plus to try integrate this to go-ipfs itself. Feel free to reject this approach, I am not very attached to this idea.
ipfs/kubo#6042